-
Notifications
You must be signed in to change notification settings - Fork 27
Fix bg_gradient lh bug #4111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bg_gradient lh bug #4111
Conversation
| get_qi_sat(thermo_params, bg_model) * | ||
| TD.latent_heat_sublim(thermo_params, t_sat) | ||
| ) / | ||
| (get_ql_sat(thermo_params, bg_model) + get_qi_sat(thermo_params, bg_model)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add an eps to make sure this is not divided by zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I thought I would be able to avoid it since we are only in that function if we have some non-zero cloud condensate. But I guess the numerics were not playing well with me - I actually need and sqrt(eps(FT)) in the denominator to make the implicit Rico single column test case work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I meant max(..., eps(FT)), but I can change it later. (or does max not work?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it change the results of any of the CI? I'm not super familiar with all the plots.
Also was wondering if it should converge to a real L_v at q=0 instead of 0, not that it should make a huge difference in the presence of any real cloud. I guess in equilibrium, condensate converges to 0 at the same time as the supersaturation, but that's less true in noneq.
sqrt(eps(Float64)) is only 1.49e-8 which is not unheard of for thin ice clouds, and if we ever ran Float32 simulations is 3e-4 which isn't insignificant (though I don't know if that's intended to be possible)
When I trialed fixing bg I just set to lh to Lv at ql+qi < eps(FT) -- I guess you could also just add +eps(FT)L_v or something similar to the numerator and denominator if you wanted it to be more numerically stable and continuous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prognostic edmf dycoms in ci changed. There are probably some other changes too. And we always run Float32. I agree we should do this more carefully. Could you work together with @trontrytel to make this better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I have an idea. We can use TD.liquid_fraction here? That way Lh is always between Lv and Ls.
szy21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one comment.
7858c8f to
1a40a8c
Compare
This PR fixes the latent heat bug discovered by @jbphyswx in our buoyancy gradients.
It would be great to rewrite parts of that file to get rid of the getter functions at the top and the bg_model, and to reduce the number of functions we define. But maybe we can do it in the next PRs.